Sorry for failing test cases! I'm submitting this only to get comments and ask questions.

This PR is related to creating Envoy bootstrap configuration according to this plan, based on the assumption I can get envoyproxy/envoy#10163 accepted.

Now instead of creating single file /config/envoy.json I need to create 3 files, e.g.

/config/envoy.json
/config/envoy-sds-auth-secret-tls-certicate.json
/config/envoy-sds-auth-secret-validation-context.json

In this PR I re-purposed the first command line argument for the target directory. That is, instead of contour bootstrap /config/envoy.json one would execute contour bootstrap /config to generate the config files to path /config

This change is backwards incompatible - bootstrap configuration generation will fail if user forgets to update the arguments of the contour bootstrap command in their deployment manifest.

Question: should we rather keep the old behavior by default and create another parameter for this?

Any other comments are of course welcome!

Now instead of creating single file /config/envoy.json I need to create 3 files

IIRC, the files are already passed in as flags. So could you just use the files specifies by the flags?

I think I did not understand how to do that. Currently we only pass one path as a target where to write the configuration file, and named arguments to point out certificates and key. Now when using SDS, there will be three configuration files (see slide no 6 for illustration). These new config files contain SDS protobuf messages with JSON serialization.

To provide paths for all three config files, maybe the command line could look something like:

contour bootstrap \
  /config/envoy.json \
  /config/envoy-sds-auth-secret-tls-certicate.json \
  /config/envoy-sds-auth-secret-validation-context.json \
  --xds-address=contour \
  --xds-port=8001 \
  --envoy-cafile=/certs/ca.crt \
  --envoy-cert-file=/certs/tls.crt \
  --envoy-key-file=/certs/tls.key

But here the position of unnamed arguments would become very critical and easy to mess up.

Alternatively I could use the current arguments

contour bootstrap \
  /config/envoy.json \
  --xds-address=contour \
  --xds-port=8001 \
  --envoy-cafile=/certs/ca.crt \
  --envoy-cert-file=/certs/tls.crt \
  --envoy-key-file=/certs/tls.key

then get dirname of /config/envoy.json and write SDS config files with hardcoded filenames into that directory. That would be backwards compatible but that does not sound too elegant either?

Of course, yet another viewpoint is that when NOT using TLS, the SDS configuration files would not need to be created at all.

I'll play with this and review next week. I agree that we should add a flag to enable this new behaviour; not sure what that should be yet.

Marking this PR stale since there has been no activity for 14 days. It will be closed if there is no activity for another 90 days.

Ping @jpeach

@tsaarni A couple of questions

• If the envoy change to watch secrets filenames lans, do we still need to split the SDS config into separate files? i.e. is this change still needed in that case?

• I think that it's worth exploring how we do on-disk bootstrap in general. I looked at Envoy layered_config, but couldn't get it to work. If we could get this to work, then it could be a useful customization point as well as rationalize the SDS config files.

• If the envoy change to watch secrets filenames lans, do we still need to split the SDS config into separate files? i.e. is this change still needed in that case?

Yes this change is needed: this is preparation for that Envoy PR. I have also some material linked in #2333 (comment)

The need to split the files comes from moving from static files to SDS resources. And the motivation to move to SDS is that it has existing logic to hot-reload the certificates.

The number of files come from the two config parameters that refer to the SDS resources (tls_certificate_sds_secret_configs and validation_context_sds_secret_config doc here).

Here is a snippet of static bootstrap that refers to two path based SDS resources:

{
  "transport_socket": {
    "name": "envoy.transport_sockets.tls",
    "typed_config": {
      "@type": "type.googleapis.com/envoy.api.v2.auth.UpstreamTlsContext",
      "common_tls_context": {
        "tls_certificate_sds_secret_configs": {
          "sds_config": {
            "path": "/path/to/envoy-sds-auth-secret-tls-certicate.json"
          }
        },
        "validation_context_sds_secret_config": {
          "sds_config": {
            "path": "/path/to/envoy-sds-auth-secret-validation-context.json"
          }
        }
      }
    }
  }
}

and the referred files look like this:

{
  "resources": [
    {
      "@type": "type.googleapis.com/envoy.api.v2.auth.Secret",
      "tls_certificate": {
        "certificate_chain": {
          "filename": "/path/to/envoy.pem"
        },
        "private_key": {
          "filename": "/path/to/envoy-key.pem"
        }
      }
    }
  ]
}

{
  "resources": [
    {
      "@type": "type.googleapis.com/envoy.api.v2.auth.Secret",
      "validation_context": {
        "trusted_ca": {
          "filename": "/path/to/internal-root-ca.pem"
        }
      }
    }
  ]
}

Maybe it could be possible to have just one file for SDS resources with two Secrets in it, and refer to that file twice from static bootstrap config. I did not test that but it still would not remove the need to split the configuration into more than single file.

The above was of course all existing logic within Envoy's protobuf based configuration and SDS code. What the Envoy PR changes is that Envoy will make an inotify subscription to path resources and the referred .pem files to pick up changes in them. Otherwise it relies to the same logic as it would have if these resources were sent via wire over xDS.

• I think that it's worth exploring how we do on-disk bootstrap in general. I looked at Envoy layered_config, but couldn't get it to work. If we could get this to work, then it could be a useful customization point as well as rationalize the SDS config files.

According to my (very limited) understanding, Envoy's runtime configuration with layered_runtimei.e. virtual file system layering is another way to configure Envoy. Possibly it was used before the protobuf based configuration was introduced? There is very little documentation or test code that I could study, but from what I can see it is not compatible with the protobuf based configuration API.

It is not 100% clear to me what is the scope of virtual file system based config. In the documentation there is example of setting /srv/runtime/current/envoy/health_check/min_interval
which relates to very limited set of runtime settings. It looks to me that it cannot cover the complete protobuf based config and therefore I suspect this cannot be used by Contour to provide customization hooks for Envoy config. Not 100% sure though!

Support for certificate rotation is now scheduled for Envoy 1.14.0

• sds: certificate hot-reload for xDS gRPC connection sds: certificate hot-reload for xDS gRPC connection envoyproxy/envoy#10163
• doc: certificate hot-reload for xDS gRPC connection doc: certificate hot-reload for xDS gRPC connection envoyproxy/envoy#10628

@tsaarni Sorry it took so long for me to get back to you.

I think that the flag we need to add is --envoy-resources-dir. If this is set, then the bootstrap is allowed to add out-of-line resources into that directory. This flag serves double-duty in that it enables out-of-line resources as well as specifying where they should be stored. Inside this directory, I think we can create directories for each resource class, i.e. sds, cds, rds. Contour can use internal names for the resources that aren't controllable by users.

This faked-up diff shows more about what I'm getting at here.

--- cmd/contour/bootstrap.go
+++ cmd/contour/bootstrap.go
@@ -28,7 +30,7 @@ func registerBootstrap(app *kingpin.Application) (*kingpin.CmdClause, *bootstrap
 	var ctx bootstrapContext
 
 	bootstrap := app.Command("bootstrap", "Generate bootstrap configuration.")
-	bootstrap.Arg("path", "Configuration file ('-' for standard output).").Required().StringVar(&ctx.path)
+	bootstrap.Arg("envoy-resources-dir", "Directory where configuration files will be written to.").StringVar(&ctx.config.ResourcesDir)
 	bootstrap.Flag("admin-address", "Envoy admin interface address.").StringVar(&ctx.config.AdminAddress)
 	bootstrap.Flag("admin-port", "Envoy admin interface port.").IntVar(&ctx.config.AdminPort)
 	bootstrap.Flag("xds-address", "xDS gRPC API address.").StringVar(&ctx.config.XDSAddress)

@@ -42,28 +44,41 @@ func registerBootstrap(app *kingpin.Application) (*kingpin.CmdClause, *bootstrap
-// doBootstrap writes an Envoy bootstrap configuration file to the supplied path.
+// doBootstrap writes an Envoy bootstrap configuration files to the supplied path.
 func doBootstrap(ctx *bootstrapContext) {
-	var out io.Writer
 
-	switch ctx.path {
-	case "-":
-		out = os.Stdout
-	default:
-		f, err := os.Create(ctx.path)
-		check(err)
+	if ctx.config.GrpcClientCert != "" || ctx.config.GrpcClientKey != "" || ctx.config.GrpcCABundle != "" {
+		// If one of the two TLS options is not empty, they all must be not empty
+		if !(ctx.config.GrpcClientCert != "" && ctx.config.GrpcClientKey != "" && ctx.config.GrpcCABundle != "") {
+			log.Fatal("You must supply all three TLS parameters - --envoy-cafile, --envoy-cert-file, --envoy-key-file, or none of them.")
+		}
 
-		out = f
+		if ctx.config.ResourcesDir != nil {
+			// TODO(jpeach): update constant names for these resource files.
+			writeConfig(path.Join(ctx.config.ResourcesDir, "sds", "xds-secrets.json"),
+				envoy.TLSCertificateSdsSecretConfig(ctx.config.GrpcClientCert, ctx.config.GrpcClientKey))
+
+			writeConfig(path.Join(ctx.config.ResourcesDir, "sds", "xds-validation.json"),
+				envoy.ValidationContextSdsSecretConfig(ctx.config.GrpcCABundle))
+		}
 
-		defer func() {
-			check(f.Close())
-		}()
 	}
 
-	m := &jsonpb.Marshaler{OrigName: true}
+	writeConfig(path.Join(ctx.config.Path, envoy.BootstrapConfigFile), envoy.Bootstrap(&ctx.config))
+}
+
+func writeConfig(filename string, config proto.Message) {
+	check(os.MkdirAll(path.Dir(filename), 0700))
 
-	check(m.Marshal(out, envoy.Bootstrap(&ctx.config)))
+	out, err := os.Create(filename)
+	check(err)
+
+	defer func() {
+		check(out.Close())
+	}()
+
+	m := &jsonpb.Marshaler{OrigName: true}
+	check(m.Marshal(out, config))
 }

--- internal/envoy/bootstrap.go
+++ internal/envoy/bootstrap.go
@@ -176,6 +264,10 @@ type BootstrapConfig struct {
 
 	// GrpcClientKey is the filename that contains a client key for secure gRPC with TLS.
 	GrpcClientKey string
+
+	// ResourcesDir is the directory where out of line Envoy resources can
+	// be placed.
+	ResourcesDir string
 }
 
 func (c *BootstrapConfig) xdsAddress() string   { return stringOrDefault(c.XDSAddress, "127.0.0.1") }

@stevesloka @youngnick PTAL at the --envoy-resources-dir suggestion above.

I updated the PR according to --envoy-resources-dir suggestion. Tests do not compile right now, but I will update them after we agree on the approach!

Why do we need different files? Why inject? I'm not following the need for this.

If the secret is rotated, we just need to know it happened and reload right?

I don't like the idea of users being able to inject their own arbitrary Envoy configuration into Contour.

@stevesloka I have added feature to Envoy that can be used to get Envoy to automatically reload its xDS gRPC certificates. Taking that feature into use requires changes to Envoy bootstrap configuration, therefore this PR is required in Contour.

I have documented the new Envoy feature here https://www.envoyproxy.io/docs/envoy/v1.14.1/configuration/security/secret#example-three-certificate-rotation-for-xds-grpc-connection

I have also depicted the solution from Contour's perspective in this slideset on page 6.

Sure bootstrap can change, but why does it need to be injected by users? In my head, we would just change the init command to generate a different bootstrap config.

This PR only changes the command to generate different bootstrap config - user has no way to inject custom configuration.

I believe @jpeach just proposed a directory layout for the new bootstrap config that could potentially also facilitate user specified customization at some later point, which is not part of this PR in any way. He was responding to my question in this comment #2333 (comment)

edit: corrected typo

Why do we need different files? Why inject? I'm not following the need for this.

Envoy watches secrets to reload them only if SDS is being used. But the only way to use SDS is to break out the secrets resources in this context is to separate files on disk.

If the secret is rotated, we just need to know it happened and reload right?

Yep, pretty much.

I don't like the idea of users being able to inject their own arbitrary Envoy configuration into Contour.

That's not being proposed.

What I'm proposing is a general approach that can facilitate secrets reloading, rather than a special case that only addresses secrets.

Over at the community meeting today I promised to add documentation links that explain why two new files are needed.

We use CommonTlsContext message to configure certificates and keys. Currently tls_certificates and validation_context attributes are used there. These are of type DataSource, which can hold either inline PEM buffer or filenames that point to PEM files.

The problem is that there is no hot-reload support when certificates and keys are configured in this way. When investigating this, it was pointed out to me that we could use SDS path based resources instead. The benefit here is that SDS logic in Envoy already knows how to hot-reload certificates and keys into active SSL context without disconnecting. Envoy also knew how to load SDS resources from a file instead of loading them from xDS server. All I needed to add was a trigger to activate this behavior when the certificate and key files changed on disk.

The complexity comes from the fact, that when using SDS resources on local path, we need to refer not only to the certificate and key files, but also to two new files: the JSON representation of the SDS resources / protobuf messages. See #2333 (comment).

So the proposed change is:

The bootstrap will now contain CommonTlsContext message with tls_certificate_sds_secret_configs and validation_context_sds_secret_config attributes. These refer to type SdsSecretConfig which uses ConfigSource message type (instead of DataSource as previously). Note that there is no "inline" alternative in the ConfigSource message, so we cannot inline the SDS resources - we need to write two new files.

There is no way for user to inject anything additional: the main bootstrap file explicitly refers to these two new files. Furthermore it refers to the files in context of tls_certificate_sds_secret_configs and validation_context_sds_secret_config attributes, which can only refer to SDS resources like shown in #2333 (comment). Envoy would reject the configuration if user would try to replace the content of these files with some other protobuf message types.

I see now, thanks for clarifying with the docs @tsaarni! The param makes sense to point to a path on the file system. I wonder if we need the parameter at all to define where the certs will live. Seems like we can default to our own, and folks could map them in as they see fit from the secret reference.

If we need to keep the param, we should call it something that makes it specific to SDS and secrets, to me, the name envoy-resources-dir seems too vague for what it's really going to be used for, but names are hard. =)

Seems like we can default to our own, and folks could map them in as they see fit from the secret reference.

How could the default configuration directory layout look?

Currently the to-be-generated bootstrap file is given as parameter now contour bootstrap /config/envoy.json but I don't know if it is based on a real use case - or could there be hardcoded layout e.g. just create all configs in /config/?

As discussed above, I think one aspect in whatever approach we take is backwards compatibility: users might upgrade Contour without studying all the detailed changes in example deployment manifest. If semantics of command line parameters change we might cause problems to users.

I wonder if we need the parameter at all to define where the certs will live.

Specifically related to certificates: the possibility to customize the certificate/key paths is important when integrating with different software that issues the certificates. For example, there Secrets created by contour certgen look different than the ones created by cert-manager. I wrote about the differences in #2149 (comment) after experimenting with cert-manager.

If we need to keep the param, we should call it something that makes it specific to SDS and secrets, to me, the name envoy-resources-dir seems too vague for what it's really going to be used for, but names are hard. =)

The reason I suggested a generic name for this was that I was concerned that we might start using the external files for more resource kinds over time. The idea behind envoy-resources-dir is that it can be a contour-managed hierarchy of arbitrary envoy resources. I didn't want to end up with extra flags each time we did this, so I thought that we could establish a pattern for doing this with envoy resources in a general way, then use that for SDS.

Okay, sorry to have not checked in here for a while.

I think the two important things to keep in mind here are:

• We need to ensure that we can rotate the certs when the secrets change. To do this, Envoy requires we do things in a certain way, which inolves multiple files. Passing a path is the cleanest way to do that.
• We need to ensure that the current behaviour - that passing a file outputs a complete config in only that file - is kept. Adding a new parameter is the cleanest way to do that, as the presence of the parameter is also a boolean indicating this new mode should be enabled.

I agree that keeping this with a more generic name is probably better, as it allows us to use this option for other things if we need to.

I don't think that we need to be concerned about people using this to pass arbitrary Envoy config, as Envoy is configured by the bootstrap. If people want to write their own bootstrap config, that's fine, and has always been intended as an extension point. If they want seamless certificate rotation, they'll need to do this as well though.

Codecov Report

Merging #2333 into master will increase coverage by 0.28%.
The diff coverage is 71.83%.

@@            Coverage Diff             @@
##           master    #2333      +/-   ##
==========================================
+ Coverage   76.57%   76.85%   +0.28%     
==========================================
  Files          68       68              
  Lines        5515     5612      +97     
==========================================
+ Hits         4223     4313      +90     
- Misses       1194     1203       +9     
+ Partials       98       96       -2